-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add routing #653
Add routing #653
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing!! I built it on my local and it looks so clean! Just had some questions that honestly we could discuss during our meeting ⭐
export const EpisodeContext = createContext({ | ||
// the default episode. | ||
episode: DEFAULT_EPISODE, | ||
setEpisode: (episode: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: should we standardize how we refer to the current episode? I have been calling it episodeId
(referring to name_short
like bc23
) but I would be OK with using episode
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should def standardize this
What if we just call name_short episodeId
and call name_long episodeName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this ^^
import { EpisodeContext } from "../../../contexts/EpisodeContext"; | ||
import { MemoryRouter } from "react-router-dom"; | ||
|
||
test('should link to default episode', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My tests are currently not working but when they are we will have dozens... thinking this may be a good opportunity to use the API/UI: <test message> (STABLE/UNSTABLE)
format for test messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('should link to default episode', () => { | |
test('UI: should link to default episode', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update after discussion: we won't include (stable/unstable), but we will include api/ui
addressed code review comments, let me know if this is good to merge! |
Looks good to me!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowell said it looked good so
adds routing and
EpisodeContext
. also adds stubs for a lot of pages